Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: deprecate usage of cursor.execute statements in favor of the in class implementation of execute. #60748

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

refactor: deprecate usage of cursor.execute statements in favor of the in class implementation of execute. #60748

wants to merge 7 commits into from
+43 −20

Conversation

gmcrocetti
Copy link

@gmcrocetti gmcrocetti commented Jan 21, 2025 •
edited
Loading

  • closes #xxxx (Replace xxxx with the GitHub issue number)
  • Tests added and passed if fixing a bug or adding a new feature
  • All code checks passed.
  • Added type annotations to new arguments/methods/functions.
  • Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

@gmcrocetti gmcrocetti force-pushed the refactor-io-sql-execute branch 5 times, most recently from 04fee59 to e9cbf63 Compare January 22, 2025 02:15
@gmcrocetti
Copy link
Author

gmcrocetti commented Jan 22, 2025 •
edited
Loading

Hello @WillAyd.
So this is the follow up of #60376.
I updated the code base to use panda's execute implementation as much as possible. I couldn't replace all places and a simple git grep '.exec' -- pandas/io/sql.py will show you that. Anyways what in my opinion is worth mentioning and asking for a review is in the following:

  1. SQLTable._execute_insert
  2. SQLTable._execute_insert_multi
  3. SQLiteTable._execute_insert
  4. SQLiteTable._execute_insert_multi

This should be no problem because we can always wrap that execution around a try-except block (as I did in SQLiteTable._execute_insert)

Would you mind taking a look and LMK what you think while in draft ?

@gmcrocetti gmcrocetti force-pushed the refactor-io-sql-execute branch from e9cbf63 to ff41294 Compare January 22, 2025 02:16
WillAyd
WillAyd reviewed Jan 22, 2025
View reviewed changes
pandas/io/sql.py Outdated Show resolved Hide resolved
@WillAyd WillAyd added Refactor Internal refactoring of code IO SQL to_sql, read_sql, read_sql_query labels Jan 22, 2025
@gmcrocetti gmcrocetti force-pushed the refactor-io-sql-execute branch 2 times, most recently from 9e0f436 to 804fb3e Compare January 22, 2025 23:55
@gmcrocetti gmcrocetti requested a review from WillAyd January 22, 2025 23:55
@gmcrocetti gmcrocetti mentioned this pull request Jan 23, 2025
Draft
5 tasks
WillAyd
WillAyd requested changes Jan 27, 2025
View reviewed changes
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment but generally this looks good. @mroeschke can you take a look as well?

pandas/io/sql.py Outdated Show resolved Hide resolved
@gmcrocetti gmcrocetti marked this pull request as ready for review February 1, 2025 23:18
@gmcrocetti gmcrocetti marked this pull request as draft February 1, 2025 23:18
WillAyd
WillAyd requested changes Feb 10, 2025
View reviewed changes
pandas/io/sql.py Outdated Show resolved Hide resolved
@gmcrocetti gmcrocetti force-pushed the refactor-io-sql-execute branch from beb8ee7 to 97939df Compare February 10, 2025 18:19
@gmcrocetti gmcrocetti force-pushed the refactor-io-sql-execute branch from ab1b946 to 71e0d24 Compare February 10, 2025 18:23
@gmcrocetti gmcrocetti requested a review from WillAyd February 10, 2025 19:25
@gmcrocetti
Copy link
Author

Hello @WillAyd ,

All tests passed here and therefore I'm opening this PR to review. I have updated the documentation :).

@gmcrocetti gmcrocetti marked this pull request as ready for review February 11, 2025 23:46
WillAyd
WillAyd reviewed Feb 12, 2025
View reviewed changes
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small comment but otherwise this lgtm. @mroeschke do you mind taking a look as well?

doc/source/whatsnew/v3.0.0.rst
@@ -76,6 +76,8 @@ Other enhancements
- Support passing a :class:`Iterable[Hashable]` input to :meth:`DataFrame.drop_duplicates` (:issue:`59237`)
- Support reading Stata 102-format (Stata 1) dta files (:issue:`58978`)
- Support reading Stata 110-format (Stata 7) dta files (:issue:`47176`)
- Refactor classes in ``pandas.io.sql`` to favor their own implementation of ``execute`` instead of relying on driver's ``execute``.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appreciate the note but we should try and make it interesting to non-developers that are just using the library. Maybe we can just combine these into:

- Errors occurring during SQL I/O will now throw a generic pandas.DatabaseError instead of the raw Exception type from the underlying driver manager library
- ```

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WillAyd WillAyd WillAyd requested changes

@mroeschke mroeschke Awaiting requested review from mroeschke

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
IO SQL to_sql, read_sql, read_sql_query Refactor Internal refactoring of code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gmcrocetti @WillAyd @mroeschke